Allow customer returns to reference existing `ReturnItem`s on create through API #4007

forkata · 2021-03-26T00:22:26Z

Description
This change fixes an issue in the API customer returns controller create action when referencing existing return items from a return authorization. This works on update but Rails has a limitation where it cannot set the customer_return_id on the ReturnItem records when they are passed as nested attributes, because that record doesn't exist. This will fail with an ActiveRecord::RecordNotFound error. This behaviour is documented, though slightly buried deep in the documentation here.

We already allow this in the CustomerReturnsController in the backend, however it requires a bit of parameter mangling to get the exact behaviour we want. In this change we have pulled the build_return_items_from_params before action from the backend controller and modified it to better fit the API use case.

The main changes required to the before_action were to

remove the code to exclude items with the returned flag, which is specific to the backend form.
allowed the nested return_items_attributes to be passed as a hash of hashes (default for nested forms) as well as an Array. I've also added a deprecation when providing these as a hash of hashes.
remove the custom errors the backend adds when a reception_status_event is not provided, this is not required by the API controller, but required in the backend.

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)
~~I have attached screenshots to this PR for visual changes (if needed)~~

api/app/controllers/spree/api/customer_returns_controller.rb

api/spec/requests/spree/api/customer_returns_controller_spec.rb

kennyadsl · 2021-03-26T08:48:19Z

@forkata Thanks for the PR!

For anyone else reviewing, here's a handy link to the backend counterpart for this feature.

remove the code to exclude items with the returned flag, which is specific to the backend form

👍

remove support for passing the return_items_attributes as a hash of hashes (default for nested forms) and instead expects that field to be an Array. We could add this back as it may be a breaking change if anyone is passing these attributes in that way through the API which the create call on an ActiveRecord object should handle. I don't think this is a documented feature in our API docs as the type is specified as array here).

Do you think there's a way to deprecate passing the hash, just to be sure we are not breaking existing applications? I'm afraid that a lot of people are looking at the source code to understand how to use our APIs, also because at the moment the doc is not super reliable as you saw.

add custom errors to the CustomerReturn and fail the create call if a reception_status_event is not provided in each the return_items_attributes. This is the behaviour in the backend controller, but I now realize reception_status_event is not required in our API docs 🤔, so I am happy to remove this change or update the docs if the field is actually required.

I'd avoid adding this validation, maybe just because I don't understand why a reception_status_event is needed. I'm open to change my mind if having a record built that way can create data inconsistencies. Did you get that?

forkata · 2021-03-26T16:57:33Z

Thank you for your quick feedback @kennyadsl!

remove support for passing the return_items_attributes as a hash of hashes (default for nested forms) and instead expects that field to be an Array. We could add this back as it may be a breaking change if anyone is passing these attributes in that way through the API which the create call on an ActiveRecord object should handle. I don't think this is a documented feature in our API docs as the type is specified as array here).

Do you think there's a way to deprecate passing the hash, just to be sure we are not breaking existing applications? I'm afraid that a lot of people are looking at the source code to understand how to use our APIs, also because at the moment the doc is not super reliable as you saw.

That is a great suggestion! I was contemplating handling both formats to ensure backwards compatibility. What I can do is add a change and corresponding tests to handle return_items_attributes as a hash of hashes and add a deprecation warning if we go down that code path.

add custom errors to the CustomerReturn and fail the create call if a reception_status_event is not provided in each the return_items_attributes. This is the behaviour in the backend controller, but I now realize reception_status_event is not required in our API docs 🤔, so I am happy to remove this change or update the docs if the field is actually required.

I'd avoid adding this validation, maybe just because I don't understand why a reception_status_event is needed. I'm open to change my mind if having a record built that way can create data inconsistencies. Did you get that?

I was also unsure if this was a backend specific requirement, but assumed it was not because of this being a feature of the state machine which is defined in the model. I am going to do some more testing on this and remove if it isn't required to create or update the CustomerReturn record or when creating new return items through this call. It would be also good to have this behaviour tested in the specs for this controller so I will try to add to that.

api/app/controllers/spree/api/customer_returns_controller.rb

forkata · 2021-03-29T20:42:58Z

Hi @kennyadsl,
After some more testing here I've updated the PR and it should be now ready for review. I made the following changes based on our conversation

the reception_status_event field appears to be optional and we can create the CR without providing that. I've added a test to show usage with and without specifying that in the payload and removed the custom error when this field was omitted.
i've added a test for passing return_items_attributes as a hash of hashes and ensured my change is backwards compatible for this, as well as added a deprecation warning in case this code path is reached. It is relatively simple to support both, but I feel like this was undocumented and probably not something we should support through the API.

kennyadsl

Thanks! Going to merge this after we release 3.0 (this will be present in 3.1).

spaghetticode

@forkata thanks for this fix, I really appreciated being guided in the review by the very detailed commits ❤️

I left a couple of comments, let me know what you think!

spaghetticode · 2021-04-09T09:03:17Z

api/app/controllers/spree/api/customer_returns_controller.rb

+          return_item.assign_attributes(item_params)
+
+          return_item
+        end.compact


I'm not sure why we need compact after the map cycle, I suppose we're always returning a Spree::ReturnItem with each iteration, and when a return item cannot be found from item_params[:id] an exception is raised... but maybe I'm overlooking some scenario?

Also, I think the naming of the method reflects only part of what this method does here. What about a more general build_customer_return, since it's actually building the instance stored in@customer_return?

I'm not sure why we need compact after the map cycle

I agree, upon closer inspection I don't think the .compact is necessary here.

I think the naming of the method reflects only part of what this method does here.

That's a great point, I think build_customer_return is a better name here, I will update that!

Thanks for the feedback, I will update this change to reflect the above suggestions!

@spaghetticode I've addressed your comments here and this should be ready for another review 🙏🏼

In a subsequent change we are going to be adding a parallel context for when we want to reference existing return items, so this is just the first step to that change. There should be no change in functionality just spacing here. Co-authored-by: Mike Conlin <mike@super.gd>

This change adds a pending test which demonstrates an issue with the `create` action when attempting to pass a reference to existing return items from a previously created return authorization. Rails does not allow us to update the association on the return items to the customer return while creating the return. Co-authored-by: Mike Conlin <mike@super.gd>

This change pulls the `before_action` from the backend customer returns controller for parsing `return_items_attributes` in order to handle creating a new customer return which references existing return items from a return authorization. This change works around a limitation in Rails when trying to update the association on existing nested resource while creating the related record. Co-authored-by: Mike Conlin <mike@super.gd>

This is a behaviour which was previously undocumented worked because of the native Rails parameter parsing for nested attributes. This is not something we want to support through the API going forward so we are adding a deprecation warning for anyone using this behaviour currently.

spaghetticode

@forkata thank you, LGTM 👍

…hashes Ref solidusio#4007

…sh of hashes Ref solidusio#4007

…hashes Ref #4007

houndci-bot reviewed Mar 26, 2021

View reviewed changes

forkata force-pushed the supergoodsoft/create-customer-return-with-existing-items branch 2 times, most recently from 34db039 to 8ba34ba Compare March 26, 2021 00:36

forkata force-pushed the supergoodsoft/create-customer-return-with-existing-items branch from 8ba34ba to 0de072f Compare March 29, 2021 19:20

forkata marked this pull request as ready for review March 29, 2021 19:33

forkata force-pushed the supergoodsoft/create-customer-return-with-existing-items branch from 0de072f to 058b856 Compare March 29, 2021 20:40

houndci-bot reviewed Mar 29, 2021

View reviewed changes

api/app/controllers/spree/api/customer_returns_controller.rb Outdated Show resolved Hide resolved

forkata force-pushed the supergoodsoft/create-customer-return-with-existing-items branch 2 times, most recently from 2d7079c to a3031d6 Compare March 29, 2021 22:40

kennyadsl approved these changes Apr 8, 2021

View reviewed changes

kennyadsl added this to the 3.1.0 milestone Apr 8, 2021

kennyadsl added release:major Breaking change on hold until next major release changelog:solidus_api Changes to the solidus_api gem labels Apr 8, 2021

spaghetticode reviewed Apr 9, 2021

View reviewed changes

forkata and others added 4 commits April 16, 2021 09:59

forkata force-pushed the supergoodsoft/create-customer-return-with-existing-items branch from a3031d6 to 00ffaf2 Compare April 16, 2021 17:13

spaghetticode approved these changes Apr 28, 2021

View reviewed changes

kennyadsl removed the release:major Breaking change on hold until next major release label Apr 30, 2021

kennyadsl merged commit 314c607 into solidusio:master Apr 30, 2021

kennyadsl added a commit to nebulab/solidus that referenced this pull request Mar 27, 2023

Remove support for passing return_items_attributes in API as hash of …

7e271a7

…hashes Ref solidusio#4007

kennyadsl added a commit to nebulab/solidus that referenced this pull request Mar 27, 2023

Remove support for passing return_items_attributes in API as hash of …

a497b48

…hashes Ref solidusio#4007

kennyadsl added a commit to nebulab/solidus that referenced this pull request Mar 27, 2023

[API] Remove support for passing return_items_attributes in API as ha…

fbbadfa

…sh of hashes Ref solidusio#4007

kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 12, 2023

[API] Remove support for passing return_items_attributes in API as ha…

d69d946

…sh of hashes Ref solidusio#4007

kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 14, 2023

[API] Remove support for passing return_items_attributes in API as ha…

d6ae667

…sh of hashes Ref solidusio#4007

kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 18, 2023

[API] Remove support for passing return_items_attributes in API as ha…

e77f031

…sh of hashes Ref solidusio#4007

kennyadsl added a commit that referenced this pull request Apr 18, 2023

Remove support for passing return_items_attributes in API as hash of …

287c801

…hashes Ref #4007

forkata deleted the supergoodsoft/create-customer-return-with-existing-items branch June 20, 2023 23:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow customer returns to reference existing `ReturnItem`s on create through API #4007

Allow customer returns to reference existing `ReturnItem`s on create through API #4007

forkata commented Mar 26, 2021 •

edited

Loading

kennyadsl commented Mar 26, 2021

forkata commented Mar 26, 2021

forkata commented Mar 29, 2021

kennyadsl left a comment

spaghetticode left a comment

spaghetticode Apr 9, 2021

forkata Apr 9, 2021 •

edited

Loading

forkata Apr 19, 2021

spaghetticode left a comment

Allow customer returns to reference existing ReturnItems on create through API #4007

Allow customer returns to reference existing ReturnItems on create through API #4007

Conversation

forkata commented Mar 26, 2021 • edited Loading

kennyadsl commented Mar 26, 2021

forkata commented Mar 26, 2021

forkata commented Mar 29, 2021

kennyadsl left a comment

Choose a reason for hiding this comment

spaghetticode left a comment

Choose a reason for hiding this comment

spaghetticode Apr 9, 2021

Choose a reason for hiding this comment

forkata Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

forkata Apr 19, 2021

Choose a reason for hiding this comment

spaghetticode left a comment

Choose a reason for hiding this comment

Allow customer returns to reference existing `ReturnItem`s on create through API #4007

Allow customer returns to reference existing `ReturnItem`s on create through API #4007

forkata commented Mar 26, 2021 •

edited

Loading

forkata Apr 9, 2021 •

edited

Loading